Skip to content

GH-50578: [C++][FlightRPC][ODBC] Always return SQL_NO_DATA from GetMoreResults - #50700

Merged
amoeba merged 2 commits into
apache:mainfrom
amoeba:flightsqlodbc-GetMoreResults
Jul 31, 2026
Merged

GH-50578: [C++][FlightRPC][ODBC] Always return SQL_NO_DATA from GetMoreResults#50700
amoeba merged 2 commits into
apache:mainfrom
amoeba:flightsqlodbc-GetMoreResults

Conversation

@amoeba

@amoeba amoeba commented Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Rationale for this change

Fixes a bug in the implementation of ODBC GetMoreResults in the FlightSQL ODBC driver. According to https://learn.microsoft.com/en-us/sql/odbc/reference/appendixes/statement-transitions?view=sql-server-ver17#sqlmoreresults, we should return SQL_NO_DATA for some states we previously were throwing another error in. This appears to be exposed by a behavior of only the Windows ODBC driver manager: GetMoreResults always gets called even for metadata queries.

What changes are included in this PR?

  • Changed implementation and test: GetMoreResults now always returns SQL_NO_DATA.

Are these changes tested?

Yes, in CI.

Are there any user-facing changes?

No.

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #50578 has no components, please add labels for components.

@amoeba amoeba mentioned this pull request Jul 29, 2026
Closed
alinaliBQ
alinaliBQ reviewed Jul 29, 2026
View reviewed changes
Comment thread cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_statement.cc
@@ -784,11 +784,7 @@ SQLRETURN ODBCStatement::GetData(SQLSMALLINT record_number, SQLSMALLINT c_type,

@alinaliBQ alinaliBQ Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI hasn't run the C++ Extra tests, so I'm not sure whether TestSQLMoreResultsWithoutQuery will fail. If the driver manager doesn't return HY010 for SQLMoreResults, you may need to update TestSQLMoreResultsWithoutQuery to expect SQL_NO_DATA on all platforms.

arrow/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc

Lines 1986 to 1995 in deed9c0

TYPED_TEST(StatementTest, TestSQLMoreResultsWithoutQuery) {
#ifdef __linux__
ASSERT_EQ(SQL_NO_DATA, SQLMoreResults(this->stmt));
#else // Windows & Mac
// Verify function sequence error state is reported when SQLMoreResults is called
// without executing any queries
ASSERT_EQ(SQL_ERROR, SQLMoreResults(this->stmt));
VerifyOdbcErrorState(SQL_HANDLE_STMT, this->stmt, kErrorStateHY010);
#endif
}

to:

TYPED_TEST(StatementTest, TestSQLMoreResultsWithoutQuery) {
  ASSERT_EQ(SQL_NO_DATA, SQLMoreResults(this->stmt));
}

edit: fixed typo

@amoeba amoeba Jul 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I pushed without the test fix to verify the test fails in CI since I don't have the tests running locally.

@amoeba amoeba added the CI: Extra: C++ Run extra C++ CI label Jul 29, 2026
@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 29, 2026
@amoeba

amoeba commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

Failed successfully in https://github.com/apache/arrow/actions/runs/30479028112/job/90668568584?pr=50700#step:15:2559. Should pass with b1399a3.

@amoeba
amoeba marked this pull request as ready for review July 30, 2026 15:47
@amoeba
amoeba requested a review from lidavidm as a code owner July 30, 2026 15:47
@amoeba
amoeba requested a review from alinaliBQ July 30, 2026 15:47
@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #50578 has no components, please add labels for components.

@amoeba

amoeba commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

CI failures all look unrelated, the ODBC tests all pass so I think that's enough for this PR. This is ready for a review.

alinaliBQ
alinaliBQ approved these changes Jul 30, 2026
View reviewed changes

@alinaliBQ alinaliBQ left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM

@github-actions github-actions Bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 30, 2026
@amoeba
amoeba merged commit fa632d1 into apache:main Jul 31, 2026
91 of 108 checks passed
@amoeba amoeba removed the awaiting merge Awaiting merge label Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lidavidm lidavidm lidavidm approved these changes
+1 more reviewer
@alinaliBQ alinaliBQ alinaliBQ approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CI: Extra: C++ Run extra C++ CI Component: C++ Component: FlightRPC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@amoeba @lidavidm @alinaliBQ